chore!: merge bitcoin#28753, partial bitcoin#28195 (bump MIN_PEER_PROTO_VERSION to 70221)
#6877
Conversation
|
kwvg
force-pushed
the
proto_bump
branch
2 times, most recently
from
0fee8f8 to
89a99e9
Compare
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/mnauth.cpp (1)
35-41: Fix clang-format issues.The CI pipeline detected clang-format differences at line 38. Please run clang-format on this section to ensure consistent code formatting.
#!/bin/bash # Verify the clang-format issue echo "Clang-format check detected formatting differences at line 38" echo "Please run: git-clang-format HEAD~1 -- src/evo/mnauth.cpp"
🧹 Nitpick comments (3)
src/net_processing.cpp (3)
652-660: New RelayInvFiltered overloads look correct and align with MIN_PEER_PROTO bumpDropping version gating and relying on CanRelay + bloom checks is consistent and simplifies call sites. No functional concerns.
- Small ergonomics: add brief docstrings noting relatedTx vs relatedTxHash semantics to prevent misuse.
2405-2427: Minor efficiency: early return on non-relay peers before locking/PeerRefYou call GetPeerRef (locks m_peer_mutex) before checking pnode->CanRelay(). Inverting the order avoids locking for block‑relay‑only or non‑relay peers.
m_connman.ForEachNode([&](CNode* pnode) { - PeerRef peer = GetPeerRef(pnode->GetId()); - if (peer == nullptr) return; - - auto tx_relay = peer->GetTxRelay(); - if (!pnode->CanRelay() || tx_relay == nullptr) { + if (!pnode->CanRelay()) { + return; + } + PeerRef peer = GetPeerRef(pnode->GetId()); + if (peer == nullptr) return; + auto tx_relay = peer->GetTxRelay(); + if (tx_relay == nullptr) { return; }
2430-2453: Apply same early-return pattern here for consistencyMirror the CanRelay pre-check to avoid unnecessary locking on non‑relay peers.
m_connman.ForEachNode([&](CNode* pnode) { - PeerRef peer = GetPeerRef(pnode->GetId()); - if (peer == nullptr) return; - - auto tx_relay = peer->GetTxRelay(); - if (!pnode->CanRelay() || tx_relay == nullptr) { + if (!pnode->CanRelay()) { + return; + } + PeerRef peer = GetPeerRef(pnode->GetId()); + if (peer == nullptr) return; + auto tx_relay = peer->GetTxRelay(); + if (tx_relay == nullptr) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
doc/release-notes-6877.md(1 hunks)src/evo/mnauth.cpp(2 hunks)src/governance/governance.cpp(0 hunks)src/governance/object.cpp(1 hunks)src/llmq/quorums.cpp(0 hunks)src/net_processing.cpp(4 hunks)src/version.h(1 hunks)test/functional/feature_llmq_simplepose.py(2 hunks)
💤 Files with no reviewable changes (2)
- src/llmq/quorums.cpp
- src/governance/governance.cpp
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/mnauth.cppsrc/governance/object.cppsrc/version.hsrc/net_processing.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/mnauth.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6877.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_llmq_simplepose.py
🧬 Code graph analysis (2)
test/functional/feature_llmq_simplepose.py (1)
test/functional/test_framework/test_framework.py (1)
start_masternode(1775-1780)
src/net_processing.cpp (4)
src/coinjoin/server.cpp (1)
inv(354-354)src/llmq/dkgsession.cpp (4)
inv(301-301)inv(609-609)inv(820-820)inv(1175-1175)src/instantsend/instantsend.cpp (1)
inv(416-416)src/spork.cpp (1)
inv(461-461)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/mnauth.cpp
[error] 38-38: Clang-format differences detected (before formatting vs after formatting). Process exited with code 1.
[error] 93-94: Clang-format differences detected (second formatting chunk). Process exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
🔇 Additional comments (8)
doc/release-notes-6877.md (1)
1-4: LGTM!The release notes entry is clear and concise, accurately documenting the MIN_PEER_PROTO_VERSION bump to 70221.
src/governance/object.cpp (1)
597-607: LGTM!The simplification correctly removes version-gating logic for governance object relay. Since MIN_PEER_PROTO_VERSION is now 70221 (GOVSCRIPT_PROTO_VERSION), all connected peers meet the minimum requirements, making the conditional relay logic unnecessary.
test/functional/feature_llmq_simplepose.py (2)
22-23: LGTM!Good practice to define MIN_PEER_PROTO_VERSION as a module-level constant with a reference to version.h, improving maintainability and alignment with the C++ codebase.
93-93: LGTM!The update correctly uses the MIN_PEER_PROTO_VERSION constant instead of the hard-coded value, improving maintainability.
src/version.h (1)
20-20: LGTM!The MIN_PEER_PROTO_VERSION bump to 70221 (GOVSCRIPT_PROTO_VERSION) is correct and aligns with the PR objective to simplify version-gating logic. Note that this is a breaking change: clients using protocol versions below 70221 will no longer be able to connect to Dash Core.
src/evo/mnauth.cpp (2)
41-41: LGTM!The standardized signHash construction is correct. With MIN_PEER_PROTO_VERSION bumped to 70221, all connected peers meet the minimum requirements, making the previous conditional logic unnecessary.
96-96: LGTM!The signHash construction correctly uses
peer.nVersion.load()and aligns with the simplified version-gating approach.src/net_processing.cpp (1)
3526-3534: All RelayInvFiltered calls use two-argument overloads
No three-argument usages remain.
|
Maybe we should defer this to after we branch off v23 |
| from test_framework.util import assert_equal, force_finish_mnsync | ||
|
|
||
| # See version.h | ||
| MIN_PEER_PROTO_VERSION = 70221 |
There was a problem hiding this comment.
We should probably use MIN_MASTERNODE_PROTO_VERSION in this test
diff --git a/test/functional/feature_llmq_simplepose.py b/test/functional/feature_llmq_simplepose.py
index a73f154ef3..0b3cfcb395 100755
--- a/test/functional/feature_llmq_simplepose.py
+++ b/test/functional/feature_llmq_simplepose.py
@@ -20,7 +20,7 @@ from test_framework.test_framework import (
from test_framework.util import assert_equal, force_finish_mnsync
# See version.h
-MIN_PEER_PROTO_VERSION = 70221
+MIN_MASTERNODE_PROTO_VERSION = 70238
class LLMQSimplePoSeTest(DashTestFramework):
def set_test_params(self):
@@ -90,7 +90,7 @@ class LLMQSimplePoSeTest(DashTestFramework):
def force_old_mn_proto(self, mn: MasternodeInfo):
self.stop_node(mn.nodeIdx)
- self.start_masternode(mn, [f"-pushversion={MIN_PEER_PROTO_VERSION}"])
+ self.start_masternode(mn, [f"-pushversion={MIN_MASTERNODE_PROTO_VERSION - 1}"])
self.connect_nodes(mn.nodeIdx, 0)
self.reset_probe_timeouts()
return False, True
PastaPastaPasta
changed the title
MIN_PEER_PROTO_VERSION to 70221MIN_PEER_PROTO_VERSION to 70221
|
Okay; I think it makes sense for us to do this for v23. The other thing, if you can do it here, let's bump the master node minimum protocol version to the latest so that we have all master nodes upgrading to v23. |
UdjinM6
dismissed
their stale review
We broke feature_txindex_compatibility.py... The test uses v0.17.0.3 (CLIENT_VERSION=170003) which has PROTOCOL_VERSION = 70219.
|
What are we going to do about this PR? We need to get this merged in to release v23 soon. |
@PastaPastaPasta @kwvg Consider alternate solution: #6882 NOTE: I'd prefer to keep compatibility as long |
Co-Authored-By: UdjinM6 <[email protected]>
kwvg
changed the title
MIN_PEER_PROTO_VERSION to 70221MIN_PEER_PROTO_VERSION to 70221)
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
knst
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/txdb.cpp (1)
38-40: Consider clarifying the legacy key documentation.The comments document legacy DB keys that may still exist in old databases. While this is helpful, the format could be clearer.
Consider this refactor for better clarity:
// Keys used in previous version that might still be found in the DB: static constexpr uint8_t DB_COINS{'c'}; -// CBlockTreeDB::DB_TXINDEX_BLOCK{'T'}; -// CBlockTreeDB::DB_TXINDEX{'t'} -// CBlockTreeDB::ReadFlag("txindex") +// Legacy txindex keys (removed in protocol version 70221): +// CBlockTreeDB::DB_TXINDEX_BLOCK{'T'} +// CBlockTreeDB::DB_TXINDEX{'t'} +// CBlockTreeDB::ReadFlag("txindex")src/net_processing.cpp (2)
2405-2428: LGTM; consider tiny early-return micro-optimization.Logic and locks are correct. Optionally, check CanRelay() before fetching tx_relay to save work.
- auto tx_relay = peer->GetTxRelay(); - if (!pnode->CanRelay() || tx_relay == nullptr) { + if (!pnode->CanRelay()) { + return; + } + auto tx_relay = peer->GetTxRelay(); + if (tx_relay == nullptr) { return; }
2430-2453: Hash-path filter check is OK; confirm semantics.Using bloom.contains(txid) only works if peers insert txids (not just scripts). Ensure callers only use this overload where filters are pre-populated accordingly, else relevant tx invs might be skipped.
Apply the same early-return micro-optimization as above:
- auto tx_relay = peer->GetTxRelay(); - if (!pnode->CanRelay() || tx_relay == nullptr) { + if (!pnode->CanRelay()) { + return; + } + auto tx_relay = peer->GetTxRelay(); + if (tx_relay == nullptr) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
doc/release-notes-6877.md(1 hunks)src/evo/mnauth.cpp(2 hunks)src/governance/governance.cpp(0 hunks)src/governance/object.cpp(1 hunks)src/init.cpp(0 hunks)src/llmq/quorums.cpp(0 hunks)src/net_processing.cpp(4 hunks)src/txdb.cpp(2 hunks)src/txdb.h(0 hunks)src/version.h(1 hunks)test/functional/feature_llmq_simplepose.py(2 hunks)test/functional/feature_txindex_compatibility.py(0 hunks)test/functional/test_runner.py(0 hunks)
💤 Files with no reviewable changes (6)
- src/governance/governance.cpp
- src/init.cpp
- test/functional/test_runner.py
- src/txdb.h
- test/functional/feature_txindex_compatibility.py
- src/llmq/quorums.cpp
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6877.md
🚧 Files skipped from review as they are similar to previous changes (3)
- test/functional/feature_llmq_simplepose.py
- src/version.h
- src/evo/mnauth.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/txdb.cppsrc/governance/object.cppsrc/net_processing.cpp
🔇 Additional comments (4)
src/governance/object.cpp (1)
597-607: LGTM! Protocol version gating correctly removed.The simplified relay logic is appropriate after bumping MIN_PEER_PROTO_VERSION to 70221. All connected peers now support the governance script protocol, eliminating the need for per-object version checks or special handling of PROPOSAL objects.
src/txdb.cpp (2)
18-20: LGTM! Proper C++20 header usage.The replacement of
<stdint.h>with C++ standard headers (<cassert>,<cstdlib>,<iterator>) is correct:
<cassert>is used forassert()(line 110)<cstdlib>is used for_Exit()(line 148)<iterator>is used forstd::next()(line 139)This aligns with C++20 best practices.
10-10: Verify if this include is still necessary.The addition of
primitives/transaction.hmay have been required for the now-removedCheckLegacyTxindexfunction. Scanning the current code, there's no apparent usage ofCTransactionor other transaction primitive types. Consider verifying whether this include is still needed.Run the following script to check for usage of types typically defined in primitives/transaction.h:
src/net_processing.cpp (1)
652-660: API change: RelayInvFiltered overloads (no minProto gating). Verify call sites.Dropping per-call minProtoVersion is fine given MIN_PEER_PROTO_VERSION enforcement. Please confirm no lingering 3‑arg usages and no remaining ISDLOCK_PROTO_VERSION gating elsewhere.
| RelayInvFiltered(inv, *std::get<CTransactionRef>(filter)); | ||
| } else if (std::holds_alternative<uint256>(filter)) { | ||
| RelayInvFiltered(inv, std::get<uint256>(filter), ISDLOCK_PROTO_VERSION); | ||
| RelayInvFiltered(inv, std::get<uint256>(filter)); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid potential null dereference of CTransactionRef.
std::get(filter) can be empty; dereferencing would crash. Guard it.
- RelayInvFiltered(inv, *std::get<CTransactionRef>(filter));
+ const auto& txref = std::get<CTransactionRef>(filter);
+ if (txref) {
+ RelayInvFiltered(inv, *txref);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RelayInvFiltered(inv, *std::get<CTransactionRef>(filter)); | |
| } else if (std::holds_alternative<uint256>(filter)) { | |
| RelayInvFiltered(inv, std::get<uint256>(filter), ISDLOCK_PROTO_VERSION); | |
| RelayInvFiltered(inv, std::get<uint256>(filter)); | |
| } else { | |
| const auto& txref = std::get<CTransactionRef>(filter); | |
| if (txref) { | |
| RelayInvFiltered(inv, *txref); | |
| } | |
| } else if (std::holds_alternative<uint256>(filter)) { | |
| RelayInvFiltered(inv, std::get<uint256>(filter)); | |
| } else { |
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 3528-3531, the code directly dereferences
std::get<CTransactionRef>(filter) which can be a null CTransactionRef and crash;
retrieve the CTransactionRef into a local variable, check it for non-null (e.g.
if (tx) ...) before dereferencing and calling RelayInvFiltered, and handle the
null case (skip relay or log/return) instead of dereferencing unconditionally.
…`GovernanceSigner`, drop `Relay()`s and use periodic relay instead, minor cleanup b51cd1d lint: apply most `clang-format` suggestions, update circular allowlist (Kittywhiskers Van Gogh) 700e069 refactor: cleanup headers and forward decls (Kittywhiskers Van Gogh) c516fd3 refactor: apply review suggestions (Kittywhiskers Van Gogh) d19e1f3 fix: add `nullptr` check before using `FindGovernanceObject()` retval (Kittywhiskers Van Gogh) 1777919 refactor: use `std::chrono` for governance time constants (Kittywhiskers Van Gogh) 4d96f5f refactor: move `DoMaintenance()` inside `Schedule()` (Kittywhiskers Van Gogh) e224991 governance: add lock annotations for `cs_relay` (Kittywhiskers Van Gogh) df589c7 governance: introduce task for relaying governance objects (Kittywhiskers Van Gogh) 3811924 governance: drop `Relay()` from `CGovernance{Object,Vote}` (Kittywhiskers Van Gogh) 63448ff refactor: abstract away parent implementation from signer (Kittywhiskers Van Gogh) 757ded3 refactor: remove need for access to private members (Kittywhiskers Van Gogh) bd6af83 refactor: separate masternode mode logic into dedicated signer class (Kittywhiskers Van Gogh) 2b74e15 refactor: move masternode mode logic to `governance/signing.cpp` (Kittywhiskers Van Gogh) fefbe27 refactor: replace `Sign` functions with direct activeman call (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6828 * Depends on #6842 * Depends on #6877 * Dependency for #6849 * To reduce the proliferation of `PeerManager` in governance logic, relaying has been centralized to `CGovernanceManager` and now done through a scheduled task that processes a queue of requests. This reduces the amount of code that needs to fetch a reference to `PeerManager` in order to queue a message for relay and brings us closer to dropping it as a member in other objects. * On test chains, the queue of requests is processed every second but on mainnet, it is processed every 5 seconds. This is because functional tests assume instantaneous relay and fail if it takes too long to relay messages (as cascading delays cause test timeout). * Likewise, to reduce the references to `CActiveMasternodeManager`, signing logic is consolidated to `SignBasic()` with the objects using simple getter/setters (or direct assignment) where applicable. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b51cd1d knst: utACK b51cd1d Tree-SHA512: cd4bba1e7314695a215a8a83e2d5319be8d2eb2e88eb42f641f6c88ca6d74ee68ced9e95475c6b1d93f5b061993efa6a195d23f61a8775f920867d9836a0dcf8
4 participants
Additional Information
Dependency for refactor: split off governance masternode-only logic to
GovernanceSigner, dropRelay()s and use periodic relay instead, minor cleanup #6838feature_txindex_compatibility.pyuses Dash Core v0.17.0.3, which hasPROTOCOL_VERSION70219(source). Due to the minimum protocol version bump, this test will no longer pass (spotted by UdjinM6!)Legacy txindex checks were introduced in bitcoin#22626 (see dash#6296, part of Dash Core v22) and the test was introduced in bitcoin#23046 (see dash#6327, part of Dash Core v22).
The migration logic itself was added in bitcoin#13033 (see dash#4178, part of Dash Core v18).
This means that it should be safe to drop the failing test altogether as we migrated databases 5 major versions ago, the databases are not funds-sensitive and we dropped support for migration a major version ago.
Breaking Changes
Clients below
70221(GOVSCRIPT_PROTO_VERSION) will not be able to connect to Dash CoreChecklist